-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
⚠️ Make catalog server serve catalog contents over HTTPS #243
Conversation
adds cert-manager as a dependency again to create self-signed certs for the catalog server Signed-off-by: everettraven <everettraven@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #243 +/- ##
=======================================
Coverage 48.84% 48.84%
=======================================
Files 8 8
Lines 434 434
=======================================
Hits 212 212
Misses 201 201
Partials 21 21 ☔ View full report in Codecov by Sentry. |
@@ -93,7 +93,7 @@ var _ = Describe("Catalog Unpacking", func() { | |||
// the ProxyGet() call below needs an explicit port value, so if | |||
// value from url.Port() is empty, we assume port 80. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// value from url.Port() is empty, we assume port 80. | |
// value from url.Port() is empty, we assume port 443. |
Looks like |
@@ -77,9 +77,11 @@ func main() { | |||
catalogdVersion bool | |||
systemNamespace string | |||
catalogServerAddr string | |||
httpExternalAddr string | |||
httpsExternalAddr string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possible I'm getting wrapped around the axle of nomenclature, but the description says that this defaults to https, and given the name it seems that the primary focus is https and I'm not clear on how they would disable it. All the relevant flags have actionable default values.
I'm not certain that we need to have an http escape hatch here, but if there isn't one can you update the PR description to say that we're moving exclusively to secured connections?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description updated
if s.ServeTLS { | ||
return s.Server.ServeTLS(s.Listener, s.CertFile, s.KeyFile) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I finally got the upstream PR to make the controller-runtime HTTP server runnable exported, so I think we should plan to drop this package when there is a release containing it
With that in mind, I would suggest setting up TLS via an explicit s.Listener
. That would also eliminate the slight weirdness with the server struct having CertFile
and KeyFile
fields that are inert when ServeTLS: false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean always creating a listener in main.go and passing that into the Server creation, conditionally enabling TLS if the cert/key exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trgeiger that is my understanding. I would also revert the changes that I made to the server.Server. I read the wrong section of the net/http package and realize now that the changes I made were unnecessary and passing in the listener is the "right" way to go here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that clears that up for me
@@ -14,5 +14,5 @@ kind: Kustomization | |||
resources: | |||
- ../crd | |||
- ../rbac | |||
- ../certmanager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break downstream for Red Hat because they use a different certificate manager on OCP clusters.
I'd suggest setting things up similar to what we did in rukpak: https://github.com/operator-framework/rukpak/tree/main/manifests
@@ -89,10 +91,12 @@ func main() { | |||
"Enabling this will ensure there is only one active controller manager.") | |||
flag.StringVar(&systemNamespace, "system-namespace", "", "The namespace catalogd uses for internal state, configuration, and workloads") | |||
flag.StringVar(&catalogServerAddr, "catalogs-server-addr", ":8083", "The address where the unpacked catalogs' content will be accessible") | |||
flag.StringVar(&httpExternalAddr, "http-external-address", "http://catalogd-catalogserver.catalogd-system.svc", "The external address at which the http server is reachable.") | |||
flag.StringVar(&httpsExternalAddr, "https-external-address", "https://catalogd-catalogserver.catalogd-system.svc", "The external address at which the http server is reachable.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are effectively removing a flag from this executable. Is this considered a breaking change? Should the http-external-address
flag be kept but... (unsure what to do with it right now).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That being said, and in addition to @grokspawn's comment. Would it be possible for this to be set to an http://
address? Is that supported? Can we run either?
flag.StringVar(&certfile, "tls-cert", "/var/certs/tls.crt", "The certificate file used for serving catalog contents over HTTPS") | ||
flag.StringVar(&keyfile, "tls-key", "/var/certs/tls.key", "The key file used for serving catalog contents over HTTPS") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These defaults could be annoying for running the binary locally outside of a container.
WDYT about:
- Making these default to empty strings
- If they are empty, run the server without TLS
- If only one is set, fail
- If both are set, load cert/key from the specified locations, and run with TLS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, do we need to think about what happens during a key rotation? We might need a file watcher to detect when the keys change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if an http://
address is specified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I can't think of a good reason for this flag to carry a full URL with a scheme. Let's just make it --external-address=<ip>:<port>
?
Moving this to a draft until I have cycles to address review comments and make changes |
Description:
server.Server
to have options for serving contents over HTTPS